Add suppress warnings & remove redudant cast quick-fix code action - #8646
Open
zielinsky wants to merge 13 commits into
Open
Add suppress warnings & remove redudant cast quick-fix code action#8646zielinsky wants to merge 13 commits into
zielinsky wants to merge 13 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Java quick fixes for redundant casts and warning suppression. Extends Java tree parsing and diagnostic metadata. Updates Java compiler options, JVM target selection, action registration, and LSP test infrastructure. ChangesJava compiler integration
Java source analysis
Redundant cast quick fix
Warning suppression quick fix
Action wiring and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LSPClient
participant CodeActionProvider
participant RemoveRedundantCast
participant SuppressWarnings
participant JavaTrees
LSPClient->>CodeActionProvider: request Java diagnostic actions
CodeActionProvider->>RemoveRedundantCast: process redundant-cast diagnostic
RemoveRedundantCast->>JavaTrees: locate type cast
JavaTrees-->>RemoveRedundantCast: return cast ranges
RemoveRedundantCast-->>CodeActionProvider: return cast-removal edit
CodeActionProvider->>SuppressWarnings: process javac warning
SuppressWarnings->>JavaTrees: locate declaration and annotations
JavaTrees-->>SuppressWarnings: return source ranges
SuppressWarnings-->>CodeActionProvider: return suppression edit
CodeActionProvider-->>LSPClient: return code actions
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
3 times, most recently
from
July 6, 2026 13:51
0d30058 to
3af8b1e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala (1)
436-452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming ambiguity: field
javaTargetis typedJvmTarget, then pattern-matched againstJavaTarget.This isn't introduced by this diff, but the changed lines make the ambiguity more visible:
javaTarget: JvmTarget(line 437) is matched withcase j: JavaTarget => j.options(line 450). Consider renaming the field (e.g.target: JvmTarget) to reduce confusion now that the surrounding PR stack explicitly distinguishesJavaTargetfromJvmTarget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala` around lines 436 - 452, The `JavaLazyCompiler` field name is ambiguous because `javaTarget` is typed as `JvmTarget` but later pattern-matched against `JavaTarget` in `newCompiler`. Rename the field to something neutral like `target` (and update its uses such as `buildTargetId` and the `javaTarget match` block) so the `JavaTarget` vs `JvmTarget` distinction is clear and the code is easier to read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala`:
- Around line 91-112: Update warningName in SuppressWarnings so it only returns
a suppressible category for javac warnings, not ERROR diagnostics; gate the
lookup on diagnostic severity before calling warningNameFrom. Also tighten
warningNameFrom to avoid substring matches by using word-boundary/whole-token
matching instead of plain text.contains, and ensure overlapping lint names are
resolved deterministically so cases like unchecked cast map to the correct
category.
---
Nitpick comments:
In
`@metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala`:
- Around line 436-452: The `JavaLazyCompiler` field name is ambiguous because
`javaTarget` is typed as `JvmTarget` but later pattern-matched against
`JavaTarget` in `newCompiler`. Rename the field to something neutral like
`target` (and update its uses such as `buildTargetId` and the `javaTarget match`
block) so the `JavaTarget` vs `JvmTarget` distinction is clear and the code is
easier to read.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64c263cf-77b1-49f1-a005-7a55602498a2
📒 Files selected for processing (7)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalamtags-interfaces/src/main/java/scala/meta/infra/FeatureFlag.javatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
💤 Files with no reviewable changes (1)
- mtags-interfaces/src/main/java/scala/meta/infra/FeatureFlag.java
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
from
July 6, 2026 16:18
3af8b1e to
b217fe0
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/src/test/scala/tests/InfraSuite.scala (1)
15-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid leaking
enabledFlagsbetween suites.SuppressWarningsLspSuite.beforeAll()mutates a shared singleton before callingsuper.beforeAll(), so a setup failure can leaveJAVAC_OPTIONSenabled for later suites. Move the toggle into a fixture/try-finallyteardown or reset the set centrally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/InfraSuite.scala` around lines 15 - 22, The shared TestingInfra.enabledFlags set is being mutated by SuppressWarningsLspSuite.beforeAll() without guaranteed cleanup, so a failed setup can leak state into later suites. Update the test setup to either use a fixture or wrap the toggle in a try-finally teardown that always removes the JAVAC_OPTIONS flag, or centralize reset logic for TestingInfra.enabledFlags so each suite starts from a clean state. Use the existing TestingInfra singleton and SuppressWarningsLspSuite.beforeAll() as the main places to apply the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit/src/test/scala/tests/InfraSuite.scala`:
- Around line 15-22: The shared TestingInfra.enabledFlags set is being mutated
by SuppressWarningsLspSuite.beforeAll() without guaranteed cleanup, so a failed
setup can leak state into later suites. Update the test setup to either use a
fixture or wrap the toggle in a try-finally teardown that always removes the
JAVAC_OPTIONS flag, or centralize reset logic for TestingInfra.enabledFlags so
each suite starts from a clean state. Use the existing TestingInfra singleton
and SuppressWarningsLspSuite.beforeAll() as the main places to apply the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e08126e6-9956-4f39-94cd-b85abb70e10d
📒 Files selected for processing (7)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalatests/unit/src/test/scala/tests/InfraSuite.scalatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (6)
- metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala
- metals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scala
- metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
- tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala
- metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
4 times, most recently
from
July 9, 2026 10:19
5e375ac to
249d4f9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala (1)
802-812: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the concurrent diagnostics-wait pattern.
The
diagnosticsPublished = server.awaitNextDiagnostics(...)assignment before_ <- server.didFocus(path)is a deliberate pattern to register the diagnostics handler before the focus event triggers publishing. A brief comment would help future readers understand why the future is created here rather than inline with<-.📝 Suggested comment
_ <- server.didOpen(path) + // Register the diagnostics handler before didFocus so diagnostics + // triggered by the focus event are captured. diagnosticsPublished = server.awaitNextDiagnostics(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala` around lines 802 - 812, The diagnostics wait is intentionally started before the focus event to avoid missing the publish, so document this concurrent pattern in the test flow around the diagnosticsPublished assignment and server.didFocus call. Add a brief comment near the awaitNextDiagnostics setup in SuppressWarningsLspSuite to explain that the handler must be registered first, then the focus event can trigger diagnostics, and keep the existing sequencing unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala`:
- Around line 802-812: The diagnostics wait is intentionally started before the
focus event to avoid missing the publish, so document this concurrent pattern in
the test flow around the diagnosticsPublished assignment and server.didFocus
call. Add a brief comment near the awaitNextDiagnostics setup in
SuppressWarningsLspSuite to explain that the handler must be registered first,
then the focus event can trigger diagnostics, and keep the existing sequencing
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67e22879-e85c-4937-a673-59bcb6707da5
📒 Files selected for processing (9)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalatests/unit/src/test/scala/tests/InfraSuite.scalatests/unit/src/test/scala/tests/UserConfigurationSuite.scalatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/src/test/scala/tests/InfraSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala
- metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala
- metals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scala
- metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
- metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
from
July 9, 2026 13:52
249d4f9 to
f79fc10
Compare
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
from
July 9, 2026 22:34
c9a3884 to
e6255ce
Compare
zielinsky
changed the title
zielinsky
marked this pull request as ready for review
August 6, 2026 20:43
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala (1)
621-719: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a runtime-JDK guard for JDK-only lint cases.
These fixtures require newer compiler diagnostics:
this-escapeneeds JDK 21, value-based class synchronization needs JDK 16, and obsoletestrictfpneeds JDK 17. On older CI runtimes,checkSuppressWarningswaits for diagnostics that do not exist. Guard these cases with a Java-versionassume, or split the suite by JDK versions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala` around lines 621 - 719, Add Java runtime-version assumptions to the JDK-dependent cases in checkSuppressWarnings: require JDK 17+ for strictfp-method, JDK 16+ for synchronization-method, and JDK 21+ for this-escape-constructor. Keep the existing fixtures and assertions unchanged, and leave the text-blocks-method case unguarded unless its diagnostics also require a version check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala`:
- Line 559: Update the Java lint options default formatting near
JavaLintOptions.default so each option value is quoted before joining, producing
a valid JSON array of strings such as ["cast","deprecation",...].
- Around line 50-60: Remove "this-escape" from the allValues used to construct
JavaLintOptions.default, while preserving it in allowed if user configuration
should still accept it. Ensure the default lint flags remain compatible with
Java 11/17.
---
Nitpick comments:
In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala`:
- Around line 621-719: Add Java runtime-version assumptions to the JDK-dependent
cases in checkSuppressWarnings: require JDK 17+ for strictfp-method, JDK 16+ for
synchronization-method, and JDK 21+ for this-escape-constructor. Keep the
existing fixtures and assertions unchanged, and leave the text-blocks-method
case unguarded unless its diagnostics also require a version check.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 42ea884d-32e3-4622-a2f0-2f9c7d541875
📒 Files selected for processing (12)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/RemoveRedundantCast.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalatests/unit/src/main/scala/tests/BaseLspSuite.scalatests/unit/src/test/scala/tests/InfraSuite.scalatests/unit/src/test/scala/tests/UserConfigurationSuite.scalatests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scalatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (6)
- metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
- tests/unit/src/test/scala/tests/UserConfigurationSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala
- tests/unit/src/test/scala/tests/InfraSuite.scala
- metals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scala
- metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
3 times, most recently
from
August 6, 2026 22:08
431f438 to
f437208
Compare
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
from
August 6, 2026 22:15
f437208 to
301267b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scala (1)
31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
lalias forCodeAction.Line 8 imports
org.eclipse.{lsp4j => l}. Line 31 uses the fully qualifiedorg.eclipse.lsp4j.CodeActioninstead. If no other reference tolexists in this file, the alias import becomes unused. The Metals build enables fatal warnings for unused imports in several modules, so this can break compilation.♻️ Proposed change
- private val onlyRemoveRedundantCast: org.eclipse.lsp4j.CodeAction => Boolean = + private val onlyRemoveRedundantCast: l.CodeAction => Boolean = _.getTitle() == RemoveRedundantCast.titleCompile this module with Metals MCP tools to confirm no unused-import warning is raised.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scala` around lines 31 - 32, Update the onlyRemoveRedundantCast type annotation to use the imported l alias instead of the fully qualified org.eclipse.lsp4j name, ensuring the existing lsp4j alias import remains used and the CodeAction predicate behavior is unchanged.Source: Coding guidelines
tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala (1)
82-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated
DeprecatedApilayout into a shared value.The same
DeprecatedApi.javalayout appears three times. Define it once as a private val and reuse it.♻️ Proposed refactor
+ private val deprecatedApiLayout = + """|/a/src/main/java/a/DeprecatedApi.java + |package a; + | + |class DeprecatedApi { + | `@Deprecated` + | static void old() {} + |} + |""".stripMarginThen pass
extraLayout = deprecatedApiLayoutin the three cases.Also applies to: 218-225, 281-288
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala` around lines 82 - 89, Extract the repeated DeprecatedApi.java content into a private shared value in SuppressWarningsLspSuite, then replace each of the three inline layouts with extraLayout = deprecatedApiLayout. Preserve the existing string contents and stripMargin behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala`:
- Around line 229-260: Update appendWarningEdit to handle empty annotation
arrays and named value forms before constructing the TextEdit: render an empty
{} as {"warning"}, and preserve value = syntax by converting scalar or array
named values into valid forms such as {value = "deprecation", "warning"}. Keep
the existing duplicate-warning check and add coverage for empty, scalar named,
and array named annotations.
---
Nitpick comments:
In
`@tests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scala`:
- Around line 31-32: Update the onlyRemoveRedundantCast type annotation to use
the imported l alias instead of the fully qualified org.eclipse.lsp4j name,
ensuring the existing lsp4j alias import remains used and the CodeAction
predicate behavior is unchanged.
In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala`:
- Around line 82-89: Extract the repeated DeprecatedApi.java content into a
private shared value in SuppressWarningsLspSuite, then replace each of the three
inline layouts with extraLayout = deprecatedApiLayout. Preserve the existing
string contents and stripMargin behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac5d9c3a-21f5-4897-b0bb-9e475fbc3e81
📒 Files selected for processing (13)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scalametals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/RemoveRedundantCast.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalatests/unit/src/main/scala/tests/BaseLspSuite.scalatests/unit/src/test/scala/tests/InfraSuite.scalatests/unit/src/test/scala/tests/UserConfigurationSuite.scalatests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scalatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (7)
- metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala
- tests/unit/src/test/scala/tests/UserConfigurationSuite.scala
- tests/unit/src/test/scala/tests/InfraSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala
- metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
- tests/unit/src/main/scala/tests/BaseLspSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
2 times, most recently
from
August 6, 2026 23:06
03d2b80 to
7d98a0b
Compare
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
from
August 9, 2026 21:00
7d98a0b to
b5dcdaf
Compare
tgodzik
requested changes
Aug 10, 2026
| if (includeAll) options | ||
| else | ||
| options.filter(option => | ||
| option == "-Xlint" || option.startsWith("-Xlint:") |
Contributor
There was a problem hiding this comment.
Why do we need to filter -Xlint options?
Member
Author
There was a problem hiding this comment.
Passing javac options to the javapc was behind a feature flag, but lint options seem useful to always include while preserving the previous behavior.
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
from
August 11, 2026 10:40
a0bbffb to
6393911
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala (2)
54-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not deduplicate actions only by title.
If a request range contains diagnostics of the same lint category on different declarations, Line 54 builds different edits and then drops all but the first action because their titles match. Deduplicate by target or edit identity instead. Add coverage with two declarations that produce the same warning category.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala` at line 54, Update the action deduplication in SuppressWarnings so actions with the same title but different target declarations or edits are preserved; deduplicate using target or edit identity instead of getTitle(). Add coverage with two declarations producing the same warning category and verify both suppression actions remain.
197-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve a trailing comma in an existing warning array.
If the source has
@SuppressWarnings({"unchecked",}), Java accepts the trailing comma. Line 204 inserts another comma before}, which produces{"unchecked",, "rawtypes"}. Emit no leading comma when the array content already ends with one. Add a trailing-comma test case.Proposed fix
} else if (isArray) { val closeBrace = insideEnd - inside.reverse.indexOf('}') - 1 + val hasTrailingComma = + value.substring(1, value.length() - 1).trim.endsWith(",") ( new l.Range( text.indexToLspPosition(closeBrace), text.indexToLspPosition(closeBrace), ), - s""", "$warningName"""", + if (hasTrailingComma) s""""$warningName"""" + else s""", "$warningName"""", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala` around lines 197 - 205, Update the array-handling branch in SuppressWarnings so the generated insertion omits the leading comma when the existing array content already ends with a comma, preventing duplicate commas while preserving normal separator insertion otherwise. Add a test covering an existing trailing-comma annotation such as `@SuppressWarnings`({"unchecked",}) and verify the resulting warning array remains valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala`:
- Line 54: Update the action deduplication in SuppressWarnings so actions with
the same title but different target declarations or edits are preserved;
deduplicate using target or edit identity instead of getTitle(). Add coverage
with two declarations producing the same warning category and verify both
suppression actions remain.
- Around line 197-205: Update the array-handling branch in SuppressWarnings so
the generated insertion omits the leading comma when the existing array content
already ends with a comma, preventing duplicate commas while preserving normal
separator insertion otherwise. Add a test covering an existing trailing-comma
annotation such as `@SuppressWarnings`({"unchecked",}) and verify the resulting
warning array remains valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf088187-bb6d-4459-b75d-6016420f7090
📒 Files selected for processing (7)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalamtags-java/src/main/scala/scala/meta/internal/jpc/JavaDiagnostics.scalatests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scalatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (4)
- metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala
- tests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scala
- tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
- metals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scala
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
3 times, most recently
from
August 13, 2026 15:22
b9eb590 to
9d92ddb
Compare
zielinsky
force-pushed
the
metalsv2/supress-warnings
branch
from
August 13, 2026 15:26
9d92ddb to
b10a7ca
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #8502
Summary by CodeRabbit
New Features
@SuppressWarningsannotations for compiler warnings.Bug Fixes